Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Deprecate sourmash.load_signatures as public API; refactor a bit. #1279

Merged
merged 12 commits into from
Feb 4, 2021

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Jan 17, 2021

Deprecate sourmash.load_signatures in favor of sourmash_args.load_file_as_signatures and sourmash_args.load_database, to support a more robust & feature-fill public API for sourmash.

Also:

Fixes #1141
Tackles #1142, except for documentation issue.
#1092 is relevant but not addressed here.

Should probably revisit PR #921 after this.

Question:

  • should we add deprecations to v3.5.x? Probably.

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@ctb ctb changed the title Load signatures 40 Deprecate sourmash.load_signatures as public API; refactor a bit. Jan 17, 2021
@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #1279 (dbc4f26) into latest (63f545b) will increase coverage by 5.26%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1279      +/-   ##
==========================================
+ Coverage   88.69%   93.96%   +5.26%     
==========================================
  Files         125       98      -27     
  Lines       18281    14677    -3604     
  Branches     1440     1438       -2     
==========================================
- Hits        16215    13791    -2424     
+ Misses       1819      640    -1179     
+ Partials      247      246       -1     
Flag Coverage Δ
python 93.96% <93.10%> (-0.01%) ⬇️
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sourmash/signature.py 89.65% <ø> (+0.23%) ⬆️
src/sourmash/__init__.py 94.44% <84.61%> (-5.56%) ⬇️
src/sourmash/commands.py 82.39% <100.00%> (+0.03%) ⬆️
src/sourmash/index.py 95.29% <100.00%> (ø)
src/sourmash/lca/command_index.py 90.37% <100.00%> (ø)
src/sourmash/sourmash_args.py 91.92% <100.00%> (ø)
tests/test_sbt.py 100.00% <100.00%> (ø)
tests/test_sourmash.py 99.41% <100.00%> (+<0.01%) ⬆️
src/core/src/sketch/hyperloglog/estimators.rs
src/core/src/from.rs
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63f545b...dbc4f26. Read the comment docs.

@ctb ctb changed the title Deprecate sourmash.load_signatures as public API; refactor a bit. [WIP] Deprecate sourmash.load_signatures as public API; refactor a bit. Jan 20, 2021
@luizirber
Copy link
Member

luizirber commented Jan 20, 2021

sourmash.sourmash_args docstring says:
"Utility functions for dealing with input args to the sourmash command line."

At this point, shouldn't we actually move functions like sourmash_args.load_file_as_signatures and sourmash_args.load_database into the sourmash module instead, since they are useful in general (especially when using notebooks) and move any remaining functions in sourmash.sourmash_args into sourmash.cli?

@ctb
Copy link
Contributor Author

ctb commented Jan 23, 2021

sourmash.sourmash_args docstring says:
"Utility functions for dealing with input args to the sourmash command line."

At this point, shouldn't we actually move functions like sourmash_args.load_file_as_signatures and sourmash_args.load_database into the sourmash module instead, since they are useful in general (especially when using notebooks) and move any remaining functions in sourmash.sourmash_args into sourmash.cli?

hmm, I am making load_file_as_signatures and load_database available at the top level, yes, but I wasn't planning on defining them in __init__.py because I like keeping that short. I'm amenable to moving them away from sourmash_args but not sure where?

The cli submodule closely mirrors the actual CLI in structure, which I like. The functions in sourmash_args, by contrast, are used extensively by the sourmash.commands module (and related), and would seem to fit better in a command_utils module than under cli. (I seem to recall we have an issue somewhere about making sourmash more usable via the Python API and providing a way to use the commands in a notebook-friendly way, but I can't find that issue now; my memory is that it's relevant here because it talks about where to put Python-usable functions.)

It's also worth noting that sourmash_args is a bit of a dogs breakfast of odds and ends...

@ctb ctb changed the title [WIP] Deprecate sourmash.load_signatures as public API; refactor a bit. [MRG] Deprecate sourmash.load_signatures as public API; refactor a bit. Jan 23, 2021
@ctb
Copy link
Contributor Author

ctb commented Jan 23, 2021

I think this is ready for review, @luizirber

@ctb ctb requested a review from luizirber February 4, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove quiet argument from sourmash.load_signatures
2 participants